-
Notifications
You must be signed in to change notification settings - Fork 254
add nested xml input format for bucket quota #6037
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
946b893 to
87b0f49
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@francoisferrand I tried the curl command provided in the doc we gave to the client :
curl -X PUT "http://localhost:8000/testb/?quota=true" --user "accessKey1:verySecretKey1" --aws-sigv4 "aws:amz:us-east-1:s3" -d '{"quota" : 32}'
In that case, curl will not set the content-type header. So I think it's tricky to modify the logic around the body parsing by using content type, because clients can't be expected to set that header so no matter what we will have to use a fallback which is basically the code that we already have with the try/catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should check the contentType if set (and default to JSON if not) ?
switch(contentType) {
case 'xml':
return parseString(requestBody, { explicitArray: false }, (xmlError, xmlData) => { [...] };
case 'json':
default:
const jsonData = JSON.parse(requestBody);
return next(null, jsonData);
}
could be a followup ticket, or just this one as it is probably simple:
- probably a good time to make this change: 9.3 already has "risky" changes, and we have time before we ship it
- should be fine if we keep JSON as fallback (according to the docs which were shared)
- esp. quota API is not widely used at the moment
- but we need to test it explicitly to avoid any surprise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes let's make that change and add it in 9.3
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
87b0f49 to
04bf212
Compare
04bf212 to
13940d6
Compare
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
| { | ||
| "name": "@zenko/cloudserver", | ||
| "version": "9.2.14", | ||
| "version": "9.3.0-preview.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wanna do this now, because this change is for the cloudserver quota client feature, and I need an image with this new code to run in the test pipeline. Otherwise can't merge the new client
| it('should update quota with XML format', async () => { | ||
| try { | ||
| const xmlQuota = '<QuotaConfiguration><Quota>3000</Quota></QuotaConfiguration>'; | ||
| await sendRequest('PUT', '127.0.0.1:8000', `/${bucket}/?quota=true`, xmlQuota); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it not be something like this:
(may be the same, but closer to how AWS documents its own api...)
| await sendRequest('PUT', '127.0.0.1:8000', `/${bucket}/?quota=true`, xmlQuota); | |
| await sendRequest('PUT', '127.0.0.1:8000', `/${bucket}/?quota`, xmlQuota); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the api is written with quota=true in the client doc, the existing tests are also using quota=true.
Upon testing it seems that all of these will work :
{bucketName}/quota=true
{bucketName}/quota=false
{bucketName}/quota=
{bucketName}/quota
But considering the existing documentation and code, I think we can keep using quota=true
1a2fe7c to
01d8776
Compare
01d8776 to
6392190
Compare
6392190 to
247b008
Compare
|
/approve |
Build failedThe build for commit did not succeed in branch improvement/CLDSRV-818 The following options are set: approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-818. Goodbye sylvainsenechal. The following options are set: approve |
Issue: CLDSRV-818